Skip to content

fix(database): escape quote characters in identifiers - #2303

Open
osbre wants to merge 2 commits into
tempestphp:3.xfrom
osbre:fix/escape-sql-identifiers
Open

fix(database): escape quote characters in identifiers#2303
osbre wants to merge 2 commits into
tempestphp:3.xfrom
osbre:fix/escape-sql-identifiers

Conversation

@osbre

@osbre osbre commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Escape quote characters in DatabaseDialect::quoteIdentifier() to prevent potential SQL injection when identifiers come from untrusted input.

The method wraps an identifier in quote characters, but previously did not escape quote characters already inside it. This meant an identifier containing a quote could end its own quoting, causing the rest of the value to be treated as SQL instead of part of the identifier:

$dialect->quoteIdentifier('ti"tle');

// before: "ti"tle"
// after:  "ti""tle"

Doubling the quote character is the escaping format expected by MySQL/SQLite and PostgreSQL. This also matches how other frameworks handle identifier quoting, including Laravel's Illuminate\Database\Grammar::wrapValue and Doctrine DBAL's AbstractPlatform::quoteSingleIdentifier.

Handling this at the dialect level means all call sites - relations, table aliases, migration statements, etc. - get the same protection without having to sanitise identifiers individually.

Testing

Added unit test at packages/database/tests/Config/DatabaseDialectTest.php.

@github-actions

github-actions Bot commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Benchmark Results

Comparison of fix/escape-sql-identifiers against 3.x (e16e5a4844f0371d61c077008155dd1cc222baf2).

Open to see the benchmark results

No benchmark changes above ±5%.

Generated by phpbench against commit 1e68f29

@xHeaven

xHeaven commented Sep 21, 2026

Copy link
Copy Markdown
Member

Hey, I think this needs a more thorough refactor, as some paths still quote things without calling the fixed method, such as HasTableAlias.php:45 and FieldStatement.php:40.

@osbre

osbre commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@xHeaven wouldn't that be a separate follow-up PR? to keep the changes small and focused

@xHeaven

xHeaven commented Sep 21, 2026

Copy link
Copy Markdown
Member

@xHeaven wouldn't that be a separate follow-up PR? to keep the changes small and focused

Good question, I'm not sure what'd be the best here. Those changes are closely related (essentially the same thing) to this one, I would personally keep them in a single PR. @brendt what's your call here?

@brendt

brendt commented Oct 1, 2026

Copy link
Copy Markdown
Member

Keep them in the same PR :)

@osbre
osbre marked this pull request as draft October 1, 2026 04:07
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

This pull request is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the Stale This issue has seen no activity for a while label Nov 1, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

This pull request was closed because it has been inactive for 1 day since being marked as stale.

@github-actions github-actions Bot closed this Nov 3, 2026
@innocenzi innocenzi reopened this Nov 3, 2026
@innocenzi innocenzi removed the Stale This issue has seen no activity for a while label Nov 3, 2026
@brendt
brendt marked this pull request as ready for review November 6, 2026 08:29
@brendt

brendt commented Nov 6, 2026

Copy link
Copy Markdown
Member

Is this ready to merge?

@xHeaven

xHeaven commented Nov 6, 2026

Copy link
Copy Markdown
Member

Note

This message is generated by an AI agent on behalf of @xHeaven.

I think the earlier concern still stands. The helper change looks correct, but HasTableAlias::quoteIdentifier() still wraps identifiers without escaping them, and FieldStatement::compile() still returns fields unquoted for SQLite. We agreed to include those fixes here.

There’s also an issue in Query::compile(): for PostgreSQL it replaces every backtick with a double quote after identifiers have been escaped. For example, an order-by identifier containing a backtick compiles like this:

-- Before Query::compile():
"name` DESC --" ASC

-- After:
"name" DESC --" ASC

These issues predate this PR, but they leave the escaping fix incomplete. Could we cover those paths too, with tests checking the final compiled SQL?

@osbre

osbre commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Yeah unfortunately it goes much deeper than this simple change. We can check how Eloquent and Doctrine solve this to get better ideas.

@xHeaven

xHeaven commented Nov 6, 2026

Copy link
Copy Markdown
Member

Yeah unfortunately it goes much deeper than this simple change. We can check how Eloquent and Doctrine solve this to get better ideas.

Let's do that, I recommend broadening this PR if needed, rather than solving just part of the issue. Do you want to work on this (I presume it'll be a bit more substantial change) or should we take over here?

@osbre

osbre commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@xHeaven Feel free to take over 👍 I no longer have the capacity to work on it

@osbre

osbre commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Last time I checked it involved refactoring more stuff to pass DatabaseDialect parameter to properly escape identifiers, or inventing a new value object that can be passed around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brendt brendt Awaiting requested review from brendt brendt is a code owner
@innocenzi innocenzi Awaiting requested review from innocenzi innocenzi is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@osbre @xHeaven @brendt @innocenzi